Node / meshtastic / Meshtastic-Android / files / .skills / code-review / SKILL.md

Displaying Rendered • View raw • Download

Perform comprehensive code reviews for T383838Meshtastic-Android, ensuring changes adhere to KMP architecture, Kotlin Multiplatform conventions, MAD standards, and CMP best practices.

These four classes account for most of the Major findings raised on recent PRs, and they recur because the compiler, T383838detekt, and T383838spotless cannot see any of them. Check them while writing the code, not only while reviewing it.

T3838380 is a legitimate reading for RSSI (0 dBm), SNR, temperature, and air-quality concentration, so a T3838380 default silently merges "no data" with a real measurement. Both directions are bugs: an absent value gets persisted and displayed as a real one, and a genuine T3838380 gets discarded by a T383838takeIf { it != 0 } guard.

• [ ] Accumulators and defaults: a field collected over time defaults to T383838null, not T3838380. Absence checks read T383838== null and presence checks T383838!= null — never T383838== 0 for either.

• [ ] Aggregates over empty sets: median/mean/min helpers return T383838T? and propagate T383838null for an empty input. A T3838380 fallback biases the result in whichever direction the comparator sorts — under the higher-is-better RSSI ordering used for ranking, an empty set's T3838380 outranks a real T383838-80 dBm; under a plain T383838min it would instead win as the smallest. Either way the missing value competes as if measured.

• [ ] Comparators: sort missing values explicitly last; do not let them fall through to a numeric default.

• [ ] No zero-guards on real scales: T383838takeIf { it != 0 } is only valid where T3838380 is genuinely impossible. On any signed or zero-inclusive scale it destroys data.

• [ ] Proto presence: when a proto field gains explicit presence, adopt the nullable accessor everywhere rather than keeping a T3838380 comparison. Fields with no presence cannot be fixed app-side — say so rather than faking it.

• [ ] Tests: a nullable numeric needs both a null case and a zero-value case. One without the other does not pin the distinction.

Reading a T383838StateFlow (or a prior T383838suspend getter), branching on it, and then issuing separate writes leaves a window where a concurrent change interleaves — so a guard can fire against state that no longer exists and clobber a user preference. T383838NotificationPrefsImpl.setGeofenceAlertOptIn (T383838core/prefs/…/notification/NotificationPrefsImpl.kt) is the reference example: it parses, mutates, caps, and writes in one T383838edit.

• [ ] Any "if the flag is X, set Y and Z" transition happens inside one T383838edit/T383838updateData lambda.

• [ ] The decision reads the block's own T383838prefs/T383838current snapshot, not a cached T383838StateFlow value from outside.

• [ ] Multi-key transitions are one T383838edit call, not several T383838scope.launch writes.

• [ ] Radio-side config mutations go through a single T383838editSettings { } transaction (see T383838AdminController.editSettings).

Seeding a fake's backing store and then asserting the value comes back passes even if the production code under test is deleted. The test must fail when the path breaks.

• [ ] Prove the path ran: assert the side effect that only the intended path produces (a call counter incremented, a request issued, a cache written) alongside the observed value.

• [ ] Don't pre-seed the answer: drive the value in through the path being tested (a gated fake response) instead of injecting it directly into the cache.

• [ ] Isolate the variable: a test for one field must not let a second differing field explain the assertion.

• [ ] Assert survivors, not just counts: for dedup/merge logic, assert the identities that remain, not the size.

• [ ] No ordering assertions under T383838Dispatchers.Unconfined: emission order is not a stable contract there — assert final state.

• [ ] A new T383838core/database/schemas/<n>.json is accompanied by an T383838(n-1)→n test in T383838core/database/src/androidHostTest/.../*MigrationTest.kt.

• [ ] The test inserts rows at the old version, migrates, and asserts row count and column values are preserved — not merely that the migration runs.

• [ ] Columns going nullable assert that pre-existing values are retained and that the new T383838NULL state is reachable (this is class A at the storage layer).

When reviewing code, meticulously verify the following categories. Flag any deviations and propose the canonical project pattern as a fix.

• [ ] No Platform Bleed: Ensure absolutely no T383838java.* or T383838android.* imports exist in T383838commonMain source sets.

• [ ] KMP Native Alternatives: Verify the use of KMP alternatives for standard JVM libraries:

• T383838java.util.concurrent.locks.* -> T383838kotlinx.coroutines.sync.Mutex

• T383838java.util.concurrent.ConcurrentHashMap -> T383838atomicfu or Mutex-guarded T383838mutableMapOf()

• T383838java.io.* -> T383838Okio (T383838BufferedSource/T383838BufferedSink)

• T383838java.util.Locale -> Kotlin T383838uppercase()/T383838lowercase() (purged from T383838commonMain)

• [ ] Coroutine Safety: Use T383838safeCatching {} from T383838core:common instead of T383838runCatching {} in coroutine/suspend contexts. T383838runCatching silently swallows T383838CancellationException, breaking structured concurrency. Keep T383838runCatching only in cleanup/teardown code (abort, close, eviction). Use T383838kotlinx.coroutines.CancellationException (not T383838kotlin.coroutines.cancellation.CancellationException).

• [ ] Shared Helpers: If T383838androidMain and T383838jvmMain contain identical pure-Kotlin logic, mandate extracting it to a shared function in T383838commonMain.

• [ ] File Naming Conflicts: For T383838expect/T383838actual declarations, ensure files sharing the same package namespace have distinct names (e.g., keep T383838expect in T383838LogExporter.kt and shared helpers in T383838LogFormatter.kt) to avoid duplicate class errors on the JVM target.

• [ ] Interface & DI Over T383838expect/T383838actual: Check that T383838expect/T383838actual is reserved for small platform primitives. Interfaces + DI should be preferred for larger capabilities.

• [ ] Compose Multiplatform Resources: Ensure NO hardcoded strings. Must use T383838core:resources (e.g., T383838stringResource(Res.string.key) or asynchronous T383838getStringSuspend(Res.string.key) for ViewModels/Coroutines). NEVER use blocking T383838getString() in a coroutine.

• [ ] String Formatting: CMP only supports T383838%N$s and T383838%N$d. Flag any float formats (T383838%N$.1f) in Compose string resources; they must be pre-formatted using T383838NumberFormatter.format() from T383838core:common. Use T383838MetricFormatter for metric-specific displays (temperature, voltage, current, percent, humidity, pressure, SNR, RSSI).

• [ ] Centralized Dialogs & Alerts: Flag inline alert-rendering logic. Mandate the use of T383838AlertHost(alertManager) or T383838SharedDialogs from T383838core:ui/commonMain.

• [ ] Placeholders: Require T383838PlaceholderScreen(name) from T383838core:ui/commonMain for unimplemented desktopApp/JVM features. No inline placeholders in feature modules.

• [ ] Adaptive Layouts: Verify use of T383838currentWindowAdaptiveInfo(supportLargeAndXLargeWidth = true) to support desktopApp/tablet breakpoints (≥ 1200dp).

• [ ] Shared Navigation Graphs: Feature navigation graphs must be defined as extension functions on T383838EntryProviderScope<NavKey> in T383838commonMain (e.g., T383838fun EntryProviderScope<NavKey>.settingsGraph(...)). Flag any graphs defined in platform-specific source sets.

• [ ] Navigation Host: Ensure T383838MeshtasticNavDisplay (from T383838core:ui/commonMain) is used as the host instead of invoking T383838NavDisplay directly. Host modules should not configure T383838entryDecorators themselves.

• [ ] ViewModel Scoping: ViewModels obtained via T383838koinViewModel() must be inside T383838entry<T> blocks to correctly tie to the backstack lifetime.

• [ ] Annotation Usage: Ensure Koin is configured via annotations (T383838@Single, T383838@Factory, T383838@KoinViewModel).

• [ ] Root Assembly: Confirm that the root Koin DI graph is only assembled in host shells (T383838app and T383838desktop).

• [ ] Ktor Strictly: Check that Ktor is used for all HTTP networking. Flag and reject any usage of OkHttp.

• [ ] HTTP Configuration: Verify timeouts and base URLs use T383838HttpClientDefaults from T383838core:network. Never hardcode timeouts in feature modules. T383838DefaultRequest sets the base URL; feature API services use relative paths.

• [ ] Image Loading (Coil): Coil must use T383838coil-network-ktor3 in host modules. Feature modules should ONLY depend on T383838libs.coil (coil-compose) and never configure fetchers.

• [ ] Room KMP: Ensure T383838factory = { MeshtasticDatabaseConstructor.initialize() } is used in T383838Room.databaseBuilder. DAOs and Entities must reside in T383838commonMain.

• [ ] Room Patterns: Verify use of T383838@Upsert for insert-or-update logic. Check for T383838LIMIT 1 on single-row queries. Flag N+1 query patterns (loops calling single-row queries) — batch with chunked T383838WHERE IN instead.

• [ ] Bluetooth (BLE): All Bluetooth communication must be routed through T383838core:ble using Kable abstractions.

• [ ] JetBrains vs. AndroidX:

• In T383838commonMain: Must use T383838jetbrains-* aliases (e.g., T383838jetbrains-lifecycle-*, T383838jetbrains-navigation3-ui).

• In T383838androidMain: Can use T383838androidx-* or T383838jetbrains-* as appropriate, but do not mix them up in T383838commonMain.

• [ ] Compose Multiplatform: Ensure T383838compose-multiplatform-* aliases are used instead of plain T383838androidx.compose in all KMP modules.

• [ ] Test Placement: New Compose UI tests must go in T383838commonTest using T383838runComposeUiTest {} from T383838androidx.compose.ui.test.v2 (not the deprecated v1 T383838androidx.compose.ui.test package) + T383838kotlin.test.Test. Do not add T383838androidTest (instrumented) tests.

• [ ] Shared Test Utilities: Test fakes, doubles, and utilities should be placed in T383838core:testing.

• [ ] Libraries: Verify usage of T383838Turbine for Flow testing, T383838Kotest for property-based testing, and T383838Mokkery for mocking.

• [ ] Robolectric Configuration: Check that Compose UI tests running via Robolectric on JVM are pinned to T383838@Config(sdk = [34]) to prevent SDK 35 compatibility issues.

Kermit is the only logging API, and on the google flavor its writers fan every call out to both Firebase Crashlytics and Datadog RUM (T383838androidApp/src/google/.../GooglePlatformAnalytics.kt). Log level is therefore a reporting decision, not just a verbosity one.

• [ ] Severity gates reporting: T383838Severity.Error/T383838Assert become a Crashlytics non-fatal (T383838shouldReportAsException, which exempts T383838CancellationException and any T383838ExpectedCondition in the cause chain) and a Datadog RUM error (T383838shouldDowngradeForDatadog, which exempts only T383838ExpectedCondition). T383838Warn and below never report in either sink, with no exceptions. Attaching a throwable at warn level is free and keeps the stack trace in the logs, so demoting costs nothing.

• [ ] Don't "unify" the two cancellation rules. Crashlytics drops T383838CancellationException because it is a crash-triage tool; Datadog keeps it because a cancellation logged at error means a call site swallowed it instead of rethrowing — broken structured concurrency, and a real bug. That asymmetry is the detector that found #6468. Likewise, neither rule unwraps the cause chain for cancellation: coroutine machinery attaches cancellations as the cause of unrelated genuine failures, and unwrapping would silently drop those reports.

• [ ] T383838Logger.e with no throwable still reports. Crashlytics synthesises an T383838Exception(message); Datadog raises a RUM error from the level alone. T383838Logger.e { "…" } is not a cheap log line.

• [ ] Expected conditions must not be reported. Bluetooth off, a permission not granted, location services off, a deliberate disconnect, a peer/broker protocol violation, a handled retry, a guard that is doing its job — these are environment states, not bugs. Reporting them buries real regressions during release triage.

• [ ] Use the T383838ExpectedCondition seam (T383838core/common/src/commonMain/.../log/ExpectedCondition.kt):

• Exception type that only ever means "the environment said no" → implement T383838ExpectedCondition and give it a stable, low-cardinality T383838expectedConditionLabel (e.g. T383838ble-scan-bluetooth-disabled). T383838BleScanStartException is the reference example.

• Exception type shared between expected and genuine failures → leave the type alone and log that call site at T383838Logger.w.

• Both sinks consult T383838shouldReportAsException(severity, throwable), so an T383838ExpectedCondition is suppressed even if some call site logs it at error. Treat that as a backstop, not a licence to log expected states at error.

• [ ] Prefer a rate over an exception. For conditions worth watching but not fixing (watchdog fired, reconnect attempt failed), emit a warn log with a stable label and track its rate in the log backend. Do not manufacture a throwable just to get a stack trace.

• [ ] Third-party log bridges: adapters that forward another library's logs into Kermit must downgrade that library's "error" level — its errors are usually operational. See T383838core/ble/.../KermitLogEngine.kt (Kable).

• [ ] New T383838Logger.e in a PR: ask what the on-call engineer would do about it. If the answer is "nothing, that's just the user's phone", it is a T383838Logger.w.

• [ ] New Dependencies: If a new reflection-heavy dependency is added (DI, serialization, JNI, ServiceLoader), verify keep rules exist in both T383838androidApp/proguard-rules.pro (R8) and T383838desktopApp/proguard-rules.pro (ProGuard). The two files must stay aligned.

• [ ] Release Smoke-Test: For dependency or ProGuard rule changes, verify T383838assembleRelease and T383838./gradlew :desktopApp:runRelease succeed.

Problems only. Every comment identifies a concrete defect with evidence in the diff. No praise, no style preferences the linters already own, no speculative design feedback, no refactoring suggestions for code the PR did not touch. A review that finds nothing says so in one line.

1. Be Specific: Cite the exact file, line, symbol, or condition. Provide a fix direction — a snippet illustrating the canonical project pattern when the fix is not obvious.

2. One problem per comment. Do not bundle several findings into one thread.

3. Reference the Docs: Cite T383838AGENTS.md and the architecture playbooks to justify a change request (e.g., "Per AGENTS.md, T383838java.io.* cannot be used in T383838commonMain; please migrate to Okio").

4. Don't repeat what's already flagged. Check existing review threads before adding a finding.

5. Enforce Build Health — only where a gap exists: If a change lands in a KMP module and the PR's only test evidence is a bare T383838./gradlew test, say so: that task is ambiguous in KMP modules and silently skips them, so the code was never exercised and T383838allTests is required. Do not append a generic build reminder to a review that has no such gap.

"There are tests" is not coverage. For each non-trivial production change, map: changed behaviour (the concrete code path) → observable surfaces (public API, protocol handling, persisted rows, DataStore, Compose state, notifications, service lifecycle, transport, MQTT, widgets, Auto, desktop, R8-shaped release behaviour) → regression risks (ordering, reconnect/retry, process death, schema compatibility with rows an older build wrote, cross-module call sites, flavor and platform differences) → the test that should exist and does not.

A bug fix needs a test that fails without the fix. An updated screenshot golden, Room schema JSON, or regenerated baseline profile proves serialisation, not behaviour. Don't demand a test category for a surface the change cannot reach.

When a type moves files or is extracted, diff the old implementation against the new one: a removed T383838override, a changed exception contract, a dropped T383838require/T383838check, a changed default parameter value, a nullability flip on a numeric field (class A), a lost T383838@Serializable/T383838@Parcelize/Koin annotation, a scope change altering instance lifetime, a changed dispatcher or T383838SharingStarted. Then verify every call site of the removed declaration still holds. Pre-existing defects that came along with the move are in scope — label them "pre-existing — good opportunity to fix during this refactor" so the author can decide on scope.

• Commit Hygiene: Squash fixup/polish/review-feedback commits before opening a PR. Each commit should represent a logical, self-contained unit of work — not a back-and-forth conversation.

• PR Descriptions: Keep PR descriptions concise and scannable. State what changed and why, not a per-commit play-by-play. Use a short summary paragraph followed by a bullet list of changes. Avoid tables, headers-per-commit, or verbose breakdowns. Reference the T383838meshtastic/firmware repo PRs for tone and style.

• PR Titles: Use conventional commit format: T383838feat(scope):, T383838fix(scope):, T383838refactor(scope):, T383838chore(scope):. Keep titles under ~72 characters.